Added lambda adapter #1777

innermatrix · 2023-10-02T23:04:00Z

Description

Lambda adapter for Grafserv (no WebSockets support)

Todo: validate that the example code works outside of Postgraphile.

Performance impact

N/A

Security impact

N/A

Checklist

My code matches the project's code style and yarn lint:fix passes.
I've added tests for the new feature, and yarn test passes.
I have detailed the new feature in the relevant documentation.
I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
If this is a breaking change I've explained why.

changeset-bot · 2023-10-02T23:04:03Z

🦋 Changeset detected

Latest commit: 82bf624

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
grafserv	Patch
@grafserv/persisted	Patch
pgl	Patch
postgraphile	Patch
graphile-build-pg	Patch
graphile-utils	Patch
graphile	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

benjie

This is a great first draft! Please try and follow conventions in the other adaptors, such as putting the declaration merged types near the top of the file, using versioning for the adaptor, etc. Thanks for doing this!

I'd also like to see a cut down version of the lambda handler that only serves GraphQL and drops the GraphiQL/event stream/etc stuff.

Adding details to the documentation about how to configure the lambda via the AWS console or similar would also be great, but isn't required to get this merged.

🙌

grafast/grafserv/package.json

grafast/grafserv/examples/example-lambda.mjs

grafast/grafserv/package.json

grafast/grafserv/src/servers/lambda/index.ts

package.json

innermatrix · 2023-10-04T04:29:45Z

I'd also like to see a cut down version of the lambda handler that only serves GraphQL and drops the GraphiQL/event stream/etc stuff.

I'd be happy to do that, but it was not clear to me what the narrowest set of cases was that I needed to handle to support bare GraphQL. I think it might be a good idea to change class GrafservBase so that it has processGraphQLRequest and processGraphiQLRequest (and possibly some other narrow request-handling methods) whose return type is narrowed down accordingly, and then the lambda server could just call processGraphQLRequest, and let the compiler enforce a narrower response.type than what I get from handleRequest. But in absence of that I'd accept a list from you of which response types I can drop if all I want to support is bare GraphQL.

benjie · 2023-10-04T09:07:59Z

Have a look at the Fastify adaptor; it adds separate route handlers for each middleware; here's the code to just do the GraphQL part:

crystal/grafast/grafserv/src/servers/fastify/v4/index.ts

Lines 186 to 192 in 85dbd6d

    
           const digest = getDigest(request, reply); 
        
           const handlerResult = await this.graphqlHandler( 
        
             normalizeRequest(digest), 
        
             this.graphiqlHandler, 
        
           ); 
        
           const result = await convertHandlerResultToResult(handlerResult); 
        
           return this.send(request, reply, result);

(You can choose to not pass the this.graphiqlHandler if you don't want the GraphQL endpoint to fall back to rendering GraphiQL.)

I think it might be a good idea to change class GrafservBase so that it has processGraphQLRequest and processGraphiQLRequest (and possibly some other narrow request-handling methods)

Indeed, it has .graphqlHandler(...), etc already 👍

crystal/grafast/grafserv/src/core/base.ts

Lines 43 to 44 in 85dbd6d

    
           public graphqlHandler!: ReturnType<typeof makeGraphQLHandler>; 
        
           public graphiqlHandler!: ReturnType<typeof makeGraphiQLHandler>;

All processRequest does is a big try/catch and some path matching logic before handing over to the relevant handler (if any). The handlers don't do path checking - so you have to be sure that you want it to run the handler beforehand.

benjie · 2023-10-04T15:36:04Z

Are you able to address the CI issues (looks like TypeScript isn't happy), or do you need guidance?

innermatrix · 2023-10-04T16:05:57Z

Are you able to address the CI issues (looks like TypeScript isn't happy), or do you need guidance?

What I need is lunch 🙂

innermatrix · 2023-10-04T17:59:02Z

Have a look at the Fastify adaptor; it adds separate route handlers for each middleware; here's the code to just do the GraphQL part:

I gave up on trying to do this for now. There's too much going on between processRequest and graphqlHandler that I don't want to refactor in this PR. For example:

processRequest calls graphqlHandler and passes its output to convertHandlerResultToResult, but the parts of convertHandlerResultToResult that are specific to graphql are not separately available, so I'd have to call convertHandlerResultToResult myself — at which point I might have as well just called processRequest
processRequest calls handleGraphQLHandlerError, which isn't exported

Given the structure of the code right now, it's far easier to keep calling processRequest and only implement the response types that are relevant to graphql, letting the others fall into a default error case.

benjie · 2023-10-04T22:16:03Z

Given the structure of the code right now, it's far easier to keep calling processRequest and only implement the response types that are relevant to graphql, letting the others fall into a default error case.

Now's the time to refactor this code; before we reach 5.0.0 - if it's hard to do right now, that hints that it needs to be reworked. As you can probably tell it's already been reworked quickly a few times, and the result is a bit of a mess - really someone needs to cleanly separate the concerns in the newest pattern and then rewrite everything to fit that. My plan was to add as many different adaptors as we can to determine what these shared needs are, and then refactor based on that so that each adaptor can get smaller and smaller.

But I totally understand if you don't have time to take on that work! Just saying you're welcome to do it if you're inclined - just keep me abreast of any large changes since they're harder to review (we may end up splitting some of them into separate PRs).

innermatrix · 2023-10-04T22:40:46Z

But I totally understand if you don't have time to take on that work! Just saying you're welcome to do it if you're inclined - just keep me abreast of any large changes since they're harder to review (we may end up splitting some of them into separate PRs).

Yeah, I'm not saying I don't want to do it, I am saying I don't want to fold it into this PR 🙂 This PR is on the critical path to adopting v5 at work, and the refactor is not.

grafast/grafserv/src/servers/lambda/v1/index.ts

grafast/website/grafserv/servers/lambda.md

benjie · 2023-10-05T09:22:36Z

I'm happy with this. Please:

Add a changeset by running yarn changeset in the root folder and following the instructions to write a description for the feature
Format the code by running yarn lint:fix in the root folder

Then we're good to go I think?

.changeset/twelve-apes-try.md

innermatrix · 2023-10-05T16:46:12Z

@benjie lgtm ty

benjie · 2023-10-05T16:50:05Z

I'll aim to release this tomorrow, but no promises.

benjie requested changes Oct 3, 2023

View reviewed changes

innermatrix mentioned this pull request Oct 4, 2023

Clarified docs re extending RequestContext for specific servers #1783

Merged

5 tasks

innermatrix requested a review from benjie October 4, 2023 15:28

innermatrix marked this pull request as ready for review October 4, 2023 15:30

innermatrix force-pushed the lambda-adapter branch 3 times, most recently from 2ac43e5 to 4aed271 Compare October 4, 2023 20:28

Added lambda adapter

310571a

innermatrix force-pushed the lambda-adapter branch from 4aed271 to 310571a Compare October 4, 2023 22:25

benjie requested changes Oct 5, 2023

View reviewed changes

Apply suggestions from code review

d1f6a75

benjie approved these changes Oct 5, 2023

View reviewed changes

docs(changeset): Added AWS lambda adapter for grafserv

a38e650

benjie reviewed Oct 5, 2023

View reviewed changes

.changeset/twelve-apes-try.md Outdated Show resolved Hide resolved

changesets: minor -> patch

82bf624

benjie merged commit f4945f0 into graphile:main Oct 5, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added lambda adapter #1777

Added lambda adapter #1777

innermatrix commented Oct 2, 2023

changeset-bot bot commented Oct 2, 2023 •

edited

Loading

benjie left a comment

innermatrix commented Oct 4, 2023

benjie commented Oct 4, 2023 •

edited

Loading

benjie commented Oct 4, 2023

innermatrix commented Oct 4, 2023

innermatrix commented Oct 4, 2023

benjie commented Oct 4, 2023

innermatrix commented Oct 4, 2023

benjie commented Oct 5, 2023

innermatrix commented Oct 5, 2023

benjie commented Oct 5, 2023

Added lambda adapter #1777

Added lambda adapter #1777

Conversation

innermatrix commented Oct 2, 2023

Description

Performance impact

Security impact

Checklist

changeset-bot bot commented Oct 2, 2023 • edited Loading

🦋 Changeset detected

benjie left a comment

Choose a reason for hiding this comment

innermatrix commented Oct 4, 2023

benjie commented Oct 4, 2023 • edited Loading

benjie commented Oct 4, 2023

innermatrix commented Oct 4, 2023

innermatrix commented Oct 4, 2023

benjie commented Oct 4, 2023

innermatrix commented Oct 4, 2023

benjie commented Oct 5, 2023

innermatrix commented Oct 5, 2023

benjie commented Oct 5, 2023

changeset-bot bot commented Oct 2, 2023 •

edited

Loading

benjie commented Oct 4, 2023 •

edited

Loading